Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

some optimizations #288

Merged

Conversation

narcoticfresh
Copy link
Contributor

Hi guys..

this PR tries to minimize the calls done by the validation. Most is quite minor, but if you're dealing with call graphs like this (check the call numbers) - even small things matter..

call graph

I could squeeze out ~100ms out with those changes, whole request is now ~1.5s, so it saved maybe ~6-7%.. I'm still on the quest to squeeze more out, but only those were the low hanging fruits I've found..

It seems there is a certain threshold where validation is 'quite quick' - if the graph becomes quite big (like in our case); there is a continuous increase in request time. Makes sense of course..

There is something I commented out in this PR where I wasn't sure why it's even there, I'll comment it separately..

// normal property verification
$this->checkUndefined($value, new \stdClass(), $path, $i);
}
//$this->checkUndefined($value, new \stdClass(), $path, $i);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't know why this was there.. what does the comment "// normal property verification" mean?

If we have no schema at all, is there any need to continue the recursion? If I check UndefinedConstraint, all it does is always checking stuff in $schema which was an empty \stdClass() all the way - anybody knows more?

@bighappyface
Copy link
Collaborator

@narcoticfresh please rebase this on master

@narcoticfresh
Copy link
Contributor Author

@bighappyface rebased and squashed - i also removed some things as suggested by earlier comments..

@bighappyface
Copy link
Collaborator

@narcoticfresh I merged a few PRs this morning and I want to be sure this wasn't affected before I merge. Would you please rebase once more? Once the tests run/pass I will merge this PR.

@narcoticfresh narcoticfresh force-pushed the feature/some-optimization branch from 4158e46 to 0015556 Compare August 19, 2016 09:52
@narcoticfresh
Copy link
Contributor Author

@bighappyface rebased & squashed again.. please merge ;-)

@bighappyface
Copy link
Collaborator

+1

@bighappyface bighappyface merged commit 0f87b31 into jsonrainbow:master Aug 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants